-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trigger column #447
Trigger column #447
Conversation
Codecov Report
@@ Coverage Diff @@
## master #447 +/- ##
==========================================
+ Coverage 82.93% 83.11% +0.18%
==========================================
Files 25 25
Lines 3556 3595 +39
Branches 821 833 +12
==========================================
+ Hits 2949 2988 +39
Misses 460 460
Partials 147 147
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did another pass through and everything is still looking reasonable to me. @MartijnR could you please look through the new failure case tests added and see if you think of any others? It'd also be great to get your thoughts on coverage for #70 and the inline question.
Here's something else I wanted to mention -- there's no support added to xform2json and little thought has been given to the JSON representation. I think in general it's up to folks who rely on those tools to participate in review or submit the functionality after the fact if they need it. |
Thank you! Will check it out and am aiming for tomorrow! |
This makes sense to me. Didn't realize JSON wasn't used internally at all. Thanks @lognaturel and @gushil! |
It is as an intermediary step. If someone actually wanted to use the JSON for something directly, they might need to introduce an index from triggers to calculations. We did that after the JSON representation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! :party: Thanks! |
Yes, of course! Thanks @lognaturel @MartijnR |
Closes #438
Closes #70